Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

refactor(toml): Clean up workspace inheritance #12971

Merged
merged 5 commits into from
Nov 17, 2023
Merged

refactor(toml): Clean up workspace inheritance #12971

merged 5 commits into from
Nov 17, 2023

Conversation

epage
Copy link
Contributor

@epage epage commented Nov 13, 2023

What does this PR try to resolve?

The goal is to simplify the code so we have a better boundary between toml/mod.rs and toml/schema.rs for when we break toml/schema.rs into a separate package for #12801.

This let us

  • Remove a trait used in some back and forth for error handling
  • Move a lot of the inheritable bookkeeping and logic out of schema.rs

A lot of these changes were inspired by cargo_toml. This included some renaming that I felt made the code clearer.

I didn't go as far as cargo_toml, yet.

  • They derive more Deserializers, producing worse errors
  • Their equivalent of InheritableField has an inherit function on it. They eagerly inherit things and then error if anything isn't inherited
    • I'm still toying with something like this but held off for now
    • One idea is InheritableField has an inherit_with function, like today, that only passes errors up but doesn't generate an error. We then have a get function that errors if it isn't inherited. We could encode the field names, for error reporting, into a type parameter for InheritableField
  • They flatten InheritableDependency into TomlDependency
    • By splitting these up, workspace.dependencies and .cargo/config.toml code can directly use TomlDependency without extra error handling
      -If we went this rout, I think I'd mergeInheritableDependency::Inherit with DetailedDependency, having callers handle the errors (much like TomlManifest is both a real and virtual)

Some things I'm trying to balance

  • Complexity
  • Quality of error messages
  • Knowing what code needs touching when changes are made
    • Some more work is needed here

How should we test and review this PR?

Additional information

@epage
refactor(toml): Switch from Workspace to Inherit terms
0d9bcd7 
`Workspace` and `MaybeWorkspace` doesn't make the intent as clear.
Generally when talking about this, we say that it "inherits".
This also better matches the terms in `cargo_toml` and `cargo-manifest`
packages.
@epage
refactor(toml): Decouple dependency and package field inheritance
58b0b18
@epage
refactor(toml): Hard-code InheritableField for package table
f8a63f6
@epage
refactor(toml): Be specific with inheriting action
0eda16f
@epage
refactor(toml): Split out InheritablePackage from InheritableFields
46d9894 
This allows us to move bookkeeping / logic out of the schema
@rustbot
Copy link
Collaborator

rustbot commented Nov 13, 2023

r? @ehuss

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added A-manifest Area: Cargo.toml issues A-workspaces Area: workspaces S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 13, 2023
weihanglo
weihanglo reviewed Nov 14, 2023
View reviewed changes
src/cargo/util/toml/schema.rs Outdated Show resolved Hide resolved
src/cargo/util/toml/mod.rs Outdated Show resolved Hide resolved
src/cargo/util/toml/schema.rs Show resolved Hide resolved
@weihanglo
Copy link
Member

This seems pretty neat!

I'd like to hand this to @Muscraft to see if they have any feedback.

r? Mustcraft

@rustbot
Copy link
Collaborator

rustbot commented Nov 14, 2023

Failed to set assignee to Mustcraft: invalid assignee

Note: Only org members with at least the repository "read" role, users with write permissions, or people who have commented on the PR may be assigned.

@weihanglo
Copy link
Member

r? Muscraft

@rustbot rustbot assigned Muscraft and unassigned ehuss Nov 14, 2023
@epage epage force-pushed the schema branch 4 times, most recently from 12b9fd4 to 46d9894 Compare November 17, 2023 18:59
Muscraft
Muscraft approved these changes Nov 17, 2023
View reviewed changes
Copy link
Member

@Muscraft Muscraft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

This makes it easier to understand and reason about what is going on!

@Muscraft
Copy link
Member

@bors r+

@bors
Copy link
Collaborator

bors commented Nov 17, 2023

📌 Commit 46d9894 has been approved by Muscraft

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 17, 2023
@bors
Copy link
Collaborator

bors commented Nov 17, 2023

⌛ Testing commit 46d9894 with merge 9765a44...

@bors
Copy link
Collaborator

bors commented Nov 17, 2023

☀️ Test successful - checks-actions
Approved by: Muscraft
Pushing 9765a44 to master...

@bors bors merged commit 9765a44 into rust-lang:master Nov 17, 2023
20 checks passed
@weihanglo weihanglo mentioned this pull request Nov 18, 2023
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 18, 2023
@bors
Auto merge of rust-lang#118037 - weihanglo:update-cargo, r=weihanglo
95a5c59 
Update cargo

11 commits in 2c03e0e2dcd05dd064fcf10cc1050d342eaf67e3..9765a449d9b7341c2b49b88da41c2268ea599720
2023-11-16 04:21:44 +0000 to 2023-11-17 20:58:23 +0000
- refactor(toml): Clean up workspace inheritance (rust-lang/cargo#12971)
- docs: Recommend a wider selection of libsecret-compatible password managers (rust-lang/cargo#12993)
- feat(cli): add color output for `cargo --list` (rust-lang/cargo#12992)
- refactor: log when loading config from file (rust-lang/cargo#12991)
- Link to rustc lint levels (rust-lang/cargo#12990)
- chore(ci): Catch naive use of AtomicU64 early (rust-lang/cargo#12988)
- cargo-credential-1password: Add missing `--account` argument to `op signin` command (rust-lang/cargo#12985)
- chore: dogfood Cargo `-Zlints` table feature (rust-lang/cargo#12178)
- cargo-credential-1password: Fix README (rust-lang/cargo#12986)
- Fix a rustflags test using a wrong buildfile name (rust-lang/cargo#12987)
- Fix some test output validation. (rust-lang/cargo#12982)

r? ghost
@epage epage mentioned this pull request Nov 19, 2023
Merged
@epage epage deleted the schema branch November 19, 2023 03:52
bors added a commit that referenced this pull request Nov 20, 2023
@bors
Auto merge of #13000 - epage:inherit, r=weihanglo
8e00f03 
refactor(toml): Further clean up inheritance

### What does this PR try to resolve?

This is a follow up to #12971 that was found as I continued working towards #12801.

The first is a more general purpose API cleanup.  I was bothered by the idea that a caller could create a `field.workspace = false` when that is disallowed, so I modified the API to prevent that.

The second is part of needing to find a home for everything in `toml/mod.rs`.  I figured `IneheritableField::as_value` is reasonable in the API, so I carried that forward.  It would be reasonable to add other methods, from an API perspective, but I left that for future exploration.

### How should we test and review this PR?

### Additional information
@ehuss ehuss added this to the 1.76.0 milestone Dec 6, 2023
@epage epage mentioned this pull request Dec 13, 2023
Closed
20 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@weihanglo weihanglo weihanglo left review comments

@Muscraft Muscraft Muscraft approved these changes

Assignees

@Muscraft Muscraft

Labels
A-manifest Area: Cargo.toml issues A-workspaces Area: workspaces S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
1.76.0
Development

Successfully merging this pull request may close these issues.
6 participants
@epage @rustbot @weihanglo @Muscraft @bors @ehuss